Add compass probe helper functions by peterbarker · Pull Request #31606 · ArduPilot/ardupilot

peterbarker · 2025-11-27T04:59:23Z

(and adjust backends to take HAL::Device rather than HAL::I2CDevice)

This is primarily to reduce the number of calls to i2c_mgr->get_device and hal.spi->get_device so moving away from OwnPtr is easier.

However, we also:

remove memory leaks when driver disabled (the check for enablement is done inside add_backend, and we don't free the allocated backend if the thing is disabled via parameter
save flash
allow for more refactoring of the probing to avoid the FOR_EACH code generation

Board                    AP_Periph  blimp  bootloader  copter  heli   iofirmware  plane  rover  sub
CubeOrange-periph-heavy  -2160             *                                                    
Durandal                            -2136  *           -2136   -2136              -2136  -2136  -2136
Hitec-Airspeed           *                 *                                                    
KakuteH7-bdshot                     -2064  *           -2064   -2072              -2064  -2064  -2064
MatekF405                           -1816  *           -1824   -1816              -1816  -1816  -1816
Pixhawk1-1M-bdshot                  -1896              -1888   -1888              -1896  -1896  -1896
SITL_x86_64_linux_gnu               *                  *       *                  *      *      *
f103-QiotekPeriph        0                 *                                                    
f303-MatekGPS            0                 *                                                    
f303-Universal           -1448             *                                                    
iomcu                                                                 *                         
revo-mini                           0      *           0       0                  0      0      0
skyviper-v2450                                         0                                        
speedybeef4                         -1816  *           -1816   -1824              -1816  -1816  -1816

peterbarker · 2025-12-08T05:07:03Z

Tested on a CubeOrange with an external HMC5883
Tested on a CUAV V6X with an external RM3100
Tested on a Durandal with an external IST8310
Tested on a CubeBlack with an external HMC5883
Tested on a CubeBlack with an external QMC5883L

peterbarker · 2025-12-08T05:10:19Z

CubeOrange-periph-heavy:

Example of OwnPtr bloat:

tridge

we need to think about how to validate this PR, tricky!

tridge · 2025-12-08T23:40:39Z

+        return;
+    }
+    auto *backend = AP_Compass_AK09916::probe_ICM20948(
+        GET_I2C_DEVICE(i2c_bus, HAL_COMPASS_AK09916_I2C_ADDR),


not using the address arguments

tridge · 2025-12-08T23:49:42Z

+
+#endif  // #if AP_COMPASS_AK09916_ENABLED && AP_COMPASS_ICM20948_ENABLED
+
+void Compass::probe_i2c_dev(DriverType driver_type, AP_Compass_Backend* (*probefn)(AP_HAL::OwnPtr<AP_HAL::Device>, bool, Rotation), uint8_t i2c_bus, uint8_t i2c_addr, bool external, Rotation rotation)


should typedef a function ptr for this, maybe AP_Compass::probe_i2c_fn_t ?

Added 3x aliases for that mess.

tridge · 2025-12-08T23:50:06Z

+    add_backend(driver_type, backend);  // add_backend does nullptr check
+}
+
+void Compass::probe_spi_dev(DriverType driver_type, AP_Compass_Backend* (*probefn)(AP_HAL::OwnPtr<AP_HAL::Device>, bool, Rotation), const char *name, bool external, Rotation rotation)


maybe AP_Compass::probe_spi_ext_fn_t ?

how does it make sense to have an external bool on an SPI device?

Good question! We will find out as more cleanups emerge. Which they will.

.... also, I don't think there's any reason you couldn't have a compass connected externally via SPI...

tridge · 2025-12-08T23:51:08Z

+
+// short-lived method which expectes a probe function that doesn't
+// offer the ability to specify an external rotation:
+void Compass::probe_spi_dev(DriverType driver_type, AP_Compass_Backend* (*probefn)(AP_HAL::OwnPtr<AP_HAL::Device>, Rotation), const char *name, Rotation rotation)


maybe AP_Compass::probe_spi_fn_t ?

Added 3x probe aliases

tpwrules

You missed DRIVER_MMC5XX3.

Other than that everything matches up. I printed the PR out on paper and checked with a pen that everything lined up! Would be happy to merge after that one is fixed.

Cleanup ideas (not mandatory):

fix the comment on LIS3MDL bus number while in there
there are several HAL_COMPASS_*_ORIENTATION_INTERNAL that can't be used e.g. QMC5883P as all_external is guaranteed true
if we use helper methods or adjust the other add_backend invocations we can fully prevent leaks and drop the enabled check in it
90% of these could be packed into a struct and iterated over in one loop. Presumably this is what you were suggesting on the call? Preserving ordering might be tricky.

peterbarker · 2025-12-09T05:50:53Z

You missed DRIVER_MMC5XX3.

Ah, late entry. Fixed.

Other than that everything matches up. I printed the PR out on paper and checked with a pen that everything lined up! Would be happy to merge after that one is fixed.

Thanks!

Cleanup ideas (not mandatory):
* fix the comment on LIS3MDL bus number while in there

Future PR. There are actually a bunch of incorrect comments floating around IIRC.

* there are several `HAL_COMPASS_*_ORIENTATION_INTERNAL` that can't be used e.g. QMC5883P as `all_external` is guaranteed true

Yes, and there's the inconsistency with the SPI compasses too which should be addressed!

* if we use helper methods or adjust the other `add_backend` invocations we can fully prevent leaks and drop the enabled check in it

In Baro I ended up with something like a probe_dev - but that was near enough to add_backend that I don't know if it will go away or not.

* 90% of these could be packed into a struct and iterated over in one loop. Presumably this is what you were suggesting on the call? Preserving ordering might be tricky.

Yes, I've done that and it does work quite nicely. I have a branch here somewhere.

I've actually tried two different flavours of that, one which uses static const structures and one that doesn't.

all_external is a blight.

... and stop leaking memory if the device isn't enabled

…_icm20948

peterbarker added the NeedsTesting label Nov 27, 2025

peterbarker added this to Peter's ArduPilot 4.8 Queue Nov 27, 2025

peterbarker force-pushed the pr/compass-helpers branch 6 times, most recently from 1fb7159 to 08b9930 Compare November 28, 2025 11:59

peterbarker added DevCallTopic and removed NeedsTesting labels Dec 8, 2025

peterbarker moved this to ReadyForDevCall in Peter's ArduPilot 4.8 Queue Dec 8, 2025

CraigElder removed the DevCallTopic label Dec 8, 2025

tridge requested changes Dec 8, 2025

View reviewed changes

peterbarker force-pushed the pr/compass-helpers branch 2 times, most recently from cb926d5 to 4630bed Compare December 9, 2025 00:26

tpwrules requested changes Dec 9, 2025

View reviewed changes

peterbarker force-pushed the pr/compass-helpers branch from 4630bed to fdf2e46 Compare December 9, 2025 05:50

peterbarker added the DevCallEU label Dec 10, 2025

peterbarker added 5 commits December 10, 2025 17:31

AP_Compass: all backends take a generic Device rather than an I2CDevice

6bdef8d

AP_Compass: add some helper functions for compass probing

2ac7e31

... and stop leaking memory if the device isn't enabled

AP_Compass: use parameters rather than constants in probe_ak09916_via…

a91c632

…_icm20948

AP_Compass: use aliases to make new methods clearer

6387296

AP_Compass: move from add_backend to probe_i2c_device for MMC5XX3

4d381a5

peterbarker force-pushed the pr/compass-helpers branch from fdf2e46 to 4d381a5 Compare December 10, 2025 06:31

tridge added the MergeOnCIPass label Dec 10, 2025

peterbarker removed the DevCallEU label Dec 10, 2025

peterbarker merged commit 096d240 into ArduPilot:master Dec 10, 2025
107 checks passed

github-project-automation Bot moved this from ReadyForDevCall to Done in Peter's ArduPilot 4.8 Queue Dec 10, 2025

peterbarker deleted the pr/compass-helpers branch December 10, 2025 10:50

peterbarker removed this from Peter's ArduPilot 4.8 Queue Dec 10, 2025


		#endif // #if AP_COMPASS_AK09916_ENABLED && AP_COMPASS_ICM20948_ENABLED

		void Compass::probe_i2c_dev(DriverType driver_type, AP_Compass_Backend* (*probefn)(AP_HAL::OwnPtr<AP_HAL::Device>, bool, Rotation), uint8_t i2c_bus, uint8_t i2c_addr, bool external, Rotation rotation)

Conversation

peterbarker commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peterbarker commented Dec 8, 2025

Uh oh!

peterbarker commented Dec 8, 2025

Uh oh!

tridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tridge Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tpwrules left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbarker commented Dec 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peterbarker commented Nov 27, 2025 •

edited

Loading

tridge Dec 8, 2025 •

edited

Loading

tpwrules left a comment •

edited

Loading